-
-
Notifications
You must be signed in to change notification settings - Fork 518
[Update] Documentation for sniff WordPress.Security.PluginMenuSlug #2592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
[Update] Documentation for sniff WordPress.Security.PluginMenuSlug #2592
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @jasonkenison! It is looking good. I left a few comments with suggestions to improve it.
> | ||
<standard> | ||
<![CDATA[ | ||
Plugin functions which can be used to add pages to the WP Admin menu should not include `__FILE__` for page registration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest expanding a bit the description. Maybe something like the suggestion below:
Plugin functions which can be used to add pages to the WP Admin menu should not include `__FILE__` for page registration. | |
WordPress functions that can be used to add pages to the WP Admin menu should not use `__FILE__` for the menu slug (or parent menu slug) parameter to avoid revealing system paths. |
I replaced "Plugin functions" with "WordPress functions" as I imagine those functions can be used in a theme as well, and I also included the reason why __FILE__
should not be used, plus a few other smaller changes. I'm not a native English speaker, so please feel free to improve my suggestion if something doesn't read very well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigoprimo I updated the description as requested. As a native English speaker, your suggestion reads clear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
<code_comparison> | ||
<code title="Valid: Slug does not include `__FILE__`."> | ||
<![CDATA[ | ||
add_menu_page( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is worth adding a second valid/invalid example with add_submenu_page()
as this is the only function that has a different signature than the others, and where __FILE__
should not be used for the parent_slug
as well as the menu_slug
?
There is no need to create new <code>
blocks, you can add the new example in the <code>
blocks that already exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigoprimo Good call. I added add_submenu_page()
examples to the existing code blocks with similar __FILE__
examples.
'My Plugin Main Page', | ||
'My Plugin', | ||
'manage_options', | ||
<em>my-plugin-main</em>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be (missing quotes)
<em>my-plugin-main</em>, | |
<em>'my-plugin-main'</em>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigoprimo You are correct, quotes are required here. Updated in my latest commit.
); | ||
|
||
add_submenu_page( | ||
'my_plugin_main_page', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonkenison, one final thing for consideration now that you added an example with the add_submenu_page()
, maybe it is worth highlighting the first parameter of this function in the valid example, and then changing the value passed to this parameter (and highlighting it as well) to use __FILE__
? This should make it clearer why an example with this function was added to the documentation (the sniff checks what is passed to both $parent_slug
and $menu_slug
instead of just $menu_slug
as it happens with the other functions).
If you are going to include an invalid example in this parameter, I would suggest adding __FILE__
plus string concatenation or some other approach to illustrate a different case that also triggers the sniff. For example, the tests for this sniff contain __FILE__ . 'parent'
(WordPress/Tests/Security/PluginMenuSlugUnitTest.inc#L9).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigoprimo agreed. I'll make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work, @jasonkenison! This is looking good.
I left just two final comments with nitpicks. Let me know if you agree or not.
'My Plugin Main Page', | ||
'My Plugin', | ||
'manage_options', | ||
<em>__FILE__ . </em>'my-plugin-subpage', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize if my original comment was unclear. What I had in mind was to keep one example with just __FILE__
, and since you were going to add another example using add_submenu_page()
, use it as an opportunity to show that the sniff is triggered as well when __FILE__
is concatenated with something else.
So, I would suggest reverting the change in this like to keep it as it was with just __FILE__
. What do you think?
<em>__FILE__ . </em>'my-plugin-subpage', | |
<em>__FILE__</em>, |
); | ||
|
||
add_submenu_page( | ||
<em>__FILE__ . </em>'my_plugin_main_page', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest highlighting just __FILE__
as the concatenation operator is not something that triggers the sniff.
<em>__FILE__ . </em>'my_plugin_main_page', | |
<em>__FILE__</em> . 'my_plugin_main_page', |
Related to #1722
Initial Docs/Security/PluginMenuSlugStandard.xml
Describes check for
__FILE__
in plugin menu slugs.